Skip to content

Identify managed platforms not tracked by a package index #2174

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
May 24, 2023
Merged

Identify managed platforms not tracked by a package index #2174

merged 11 commits into from
May 24, 2023

Conversation

alessio-perugini
Copy link
Contributor

@alessio-perugini alessio-perugini commented May 9, 2023 •
edited
Loading

Please check if the PR fulfills these requirements

See how to contribute

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

We're introducing a warning message in the cli, when the user runs the upgrade of a platform that's no longer present in the additional-urls config.
We also modify a bit some proto messages.

What is the current behavior?

Right now when a platform is installed but the URL tracking the latest update for that platform gets removed from the config we're no longer to track the newest versions, and the user is not notified about it.

What is the new behavior?

When a platform is installed but the additional URL is no longer present In the config, we send a warning message in the stdout when the user tries to perform a core upgrade for that platform.

Protobuff changes:

  • message Platform:

    • added bool indexed = 13;
    • added bool missing_metadata = 14;

  • message PlatformUpgradeResponse:

    • added Platform platform = 3;

The rpc call PlatformUpgrade now also returns the Platform object which FE can use the indexed and missing_metadata to display some warnings. Like:

  • indexed = false -> warning
  • indexed = false AND missing_metdata = true -> warning

bonus:

  • missing_metadata = true -> we can decide to show a warning because the platform could be installed with the Arduino IDE 1.8

Does this PR introduce a breaking change, and is titled accordingly?

nope

Other information

I think we should discuss if we need a system to track down various warning messages.

@alessio-perugini alessio-perugini linked an issue May 9, 2023 that may be closed by this pull request
Identify managed platforms not tracked by a package index #1768
Closed
3 tasks
@alessio-perugini alessio-perugini changed the title Identify managed platforms not tracked by a package index [draft] Identify managed platforms not tracked by a package index May 9, 2023
@alessio-perugini alessio-perugini force-pushed the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch from 2306d9e to 25ba5c8 Compare May 9, 2023 09:52
@codecov
Copy link

codecov bot commented May 9, 2023 •
edited
Loading

Codecov Report

Patch coverage: 73.07% and project coverage change: +0.80 🎉

Comparison is base (855c238) 62.53% compared to head (07b6905) 63.34%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2174      +/-   ##
==========================================
+ Coverage   62.53%   63.34%   +0.80%     
==========================================
  Files         223      223              
  Lines       19488    19663     +175     
==========================================
+ Hits        12187    12455     +268     
+ Misses       6210     6123      -87     
+ Partials     1091     1085       -6
Flag Coverage Δ
unit 63.34% <73.07%> (+0.80%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
commands/board/search.go 0.00% <0.00%> (ø)
commands/daemon/daemon.go 27.77% <33.33%> (+2.16%) ⬆️
arduino/cores/packagemanager/install_uninstall.go 55.40% <37.50%> (+0.98%) ⬆️
commands/instances.go 58.45% <57.89%> (+0.10%) ⬆️
internal/cli/core/upgrade.go 77.14% <70.00%> (+1.73%) ⬆️
commands/core/upgrade.go 80.64% <93.33%> (+30.64%) ⬆️
arduino/cores/cores.go 77.00% <100.00%> (+0.71%) ⬆️
arduino/cores/packageindex/index.go 90.33% <100.00%> (+0.29%) ⬆️
commands/board/listall.go 91.13% <100.00%> (+0.23%) ⬆️
commands/core.go 100.00% <100.00%> (ø)

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@alessio-perugini alessio-perugini force-pushed the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch from 25ba5c8 to d33e77d Compare May 11, 2023 06:54
@alessio-perugini alessio-perugini self-assigned this May 11, 2023
@alessio-perugini alessio-perugini added the type: enhancement Proposed improvement label May 11, 2023
@alessio-perugini alessio-perugini force-pushed the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch 3 times, most recently from 799e3a5 to c420bc0 Compare May 12, 2023 13:38
@alessio-perugini alessio-perugini force-pushed the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch from c420bc0 to 621052d Compare May 12, 2023 13:54
@alessio-perugini alessio-perugini changed the title [draft] Identify managed platforms not tracked by a package index Identify managed platforms not tracked by a package index May 12, 2023
@alessio-perugini alessio-perugini force-pushed the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch 2 times, most recently from 9ecb877 to 605ac83 Compare May 12, 2023 14:46
@alessio-perugini alessio-perugini requested a review from cmaglie May 12, 2023 15:09
@alessio-perugini alessio-perugini marked this pull request as ready for review May 12, 2023 15:09
cmaglie
cmaglie requested changes May 22, 2023
View reviewed changes
Copy link
Member

@cmaglie cmaglie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nitpicks, the PR looks good overall.

internal/cli/core/upgrade.go Outdated
if response == nil || response.Platform == nil {
return
}
if !response.Platform.Indexed || (response.Platform.MissingMetadata && !response.Platform.Indexed) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!Indexed || (MissingMetada && !Indexed) is redundant you can just write: !Indexed.

BTW, the case of MissingMetadata && !Indexed is still valid and should print something along the line of:
Platform %s is missing installation metadata, it may not work properly or need re-installation.
but this should not be printed in the "core update" or "core install". The question is then, where to show it? maybe on compile or upload... Anyway, since all the required information is now available to the IDE via gRPC, I think we could merge this PR as-is and think about where to display the warning in a separate issue.

@alessio-perugini alessio-perugini force-pushed the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch from 0382c1f to c184f20 Compare May 22, 2023 14:03
@alessio-perugini alessio-perugini requested a review from cmaglie May 22, 2023 15:40
@alessio-perugini alessio-perugini added this to the Arduino CLI 0.33.0 milestone May 24, 2023
@alessio-perugini alessio-perugini merged commit 493fa83 into master May 24, 2023
@alessio-perugini alessio-perugini deleted the 1768-identify-managed-platforms-not-tracked-by-a-package-index branch May 24, 2023 13:20
@per1234 per1234 mentioned this pull request Jun 6, 2023
Open
3 tasks
@per1234 per1234 added the topic: code Related to content of the project itself label Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmaglie cmaglie cmaglie approved these changes

Assignees

@alessio-perugini alessio-perugini

Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Milestone
Arduino CLI 0.33.0
Development

Successfully merging this pull request may close these issues.

Identify managed platforms not tracked by a package index
3 participants
@alessio-perugini @cmaglie @per1234